Let's add the dependency as a separate small prep commit. See db44a9b for a recent example.

nit:

What's the reason for moving this out of model.dart and into initial_snapshot.dart?

My inclination would be to keep it in model.dart. As this PR shows, this type appears in events as well as in the initial snapshot. I think of this api/model/model.dart as the place for types that appear across multiple areas of the API, whereas those other two api/model/ files events.dart and initial_snapshot.dart are for types that appear specifically in events and in the initial snapshot respectively. (And types that are specific to other particular endpoints generally go in the respective api/route/*.dart files.)

I didn't realize there were other structures in model.dart that lived in two places (although that is clear now). I only moved it thinking that it was initially constructed in initial_snapshot.dart. Moving it back here.

Hmm yeah, moving this to come after User is a good idea — the rest of this file is organized in general-to-specific order, so good to match that here.

Let's do that in its own commit, though, or I guess combined with adding that handy line of dartdoc, and perhaps combined with any other code moves of a similar nature. That commit can then also be labeled as NFC.

Let's separate out adding this enum as its own commit.

(I.e. adding the class, changing this field to use it, and other closely-connected changes like in RealmUserUpdateEvent, as one commit; but separate from adding the CustomProfileFieldsEvent type, moving the definition of ProfileFieldUserData, adding the new UI, and so on.)

This TODO comment can point to the relevant issue:

I think this condition would be appropriate to push down inside ProfileDataTable.build, simplifying this code in the parent.

This and the rest of the localTimeWidget logic can probably best be moved into a little widget of their own, as the work they're doing is pretty independent of the rest of this method.

As a bonus, that would set us up to make an enhancement where that widget would rebuild every time the answer changes (so at the start of each minute), instead of getting out of date if the screen stays open for a while and then you come back to it. That'd be neat. (Wouldn't be worth tons of effort; but my guess is that it's pretty straightforward once one lays hands on the right API.)

It looks like this is basically testing the fromJson method, and so checking that e.g. the name field it gets equals the value of the name property in the JSON.

That means that when it says equals('site'), the fact that 'site' is the same value as appears in the input JSON is an essential part of the test. Because it's essential to the test, it's best if that fact can be found within the test case's own source code. For example like this:

Where the baseJson pattern comes in handy, on the other hand (as well as other ways of expressing default/baseline test data outside an individual test), is for all the data whose specifics are irrelevant to a given test, where the test just needs something there that makes the data well-formed.

So for example up at the top of this file:

    test('delivery_email', () {
      check(mkUser({'delivery_email': 'name@email.com'}).deliveryEmailStaleDoNotUse)
        .equals('name@email.com');

that test doesn't care about any of the other properties in the input JSON, except that they have to be there and have appropriate types so that fromJson doesn't throw. So the baseJson encoded in mkUser serves it very well for those. It only cares about delivery_email — and so it specifies that one property explicitly.

We could have written that test like this:

    test('delivery_email', () {
      check(mkUser({}).deliveryEmailStaleDoNotUse)
        .equals('name@example.com');

relying on the delivery_email that's supplied in baseJson. But then the test wouldn't be self-contained — for the reader to understand what the test is trying to say, they'd need to go consult what's in baseJson. So instead we make the intended delivery_email value explicit.

For this particular test, it's really just testing the auto-generated code for fromJson, because we happily have no interesting customizations on this type. So what I'd therefore actually prefer to do is to leave the test out.

Where we do have customizations of our own, especially any nontrivial ones, it's good to test those specifically. That's what you see happening earlier in this file on e.g. User — we test the handling of profileData and isSystemBot, which involve code of our own. Also of deliveryEmailStaleDoNotUse, which is a bit more marginal but does have our own customization in the form of that JsonKey annotation.

This will throw when the other user is the self-user.

Probably a good way to handle this is to add another constructor on DmNarrow, something like

  factory DmNarrow.withUser(int userId, {required int selfUserId}) {
    // …

(or maybe make userId a named parameter too).

Then that can encapsulate this logic, plus the case where the user is self.

(You asked about this in our video call, and our plan is to file a separate issue for it and leave it out of the initial version of this profile screen. Previous related discussion at #287 (comment) .)

This is fine, but to be clear (and in case it feels lighter-weight to you), what I had in mind at #287 (comment) was more like

• have a function that's just this switch statement, returning a String;
• then ProfilePage.build can have a Text widget directly in the widget tree that it returns.

I was in the mode of making everything consistently a widget, but it doesn't feel like a necessity here. I do prefer how lighter weight making this a function becomes.

I wonder how much this adds to startup time.

If it's more than like a couple of milliseconds, it may be better to defer it to be done lazily.

So I've run it a few times using the Stopwatch helper and it appears to take between 80-100 milliseconds (for comparison, LicenseRegistry.addLicense is generally 0 and LiveZulipBinding.ensureInitialized is ~2). I think we'll need to figure out a way to optimize this.

Dang. Yeah, I agree.

For cross-reference, chat thread here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/timezone.20initialization.20time/near/1633575

(And we ended up splitting this feature out as its own issue: #292.)

nit: put the fields in the same order here on the constructor as their declarations

If the type is one we don't know about, this will cheerfully carry on and show the raw contents (which are likely some kind of JSON blob) to the user as if the other user had typed that in as plain text. That's potentially pretty odd-looking.

Looks like the zulip-mobile implementation handles this case by skipping that custom profile field entirely, which I think is pretty reasonable. The most thorough thing would I guess be to instead explicitly say that it's there and that this app doesn't support it.

Let's add a test for a user with a long name — I expect it'll throw an error for overflow in this Row.

Neat! I don't think I've seen this widget before.

How does TableCellVerticalAlignment.baseline behave when one cell has multiple lines of text? If we can get basically top alignment, but with the baselines of the first lines instead of the top edges, that seems ideal.

Using TableCellVerticalAlignment.baseline with TextBaseline.alphabetic still maintains top alignment, so the ideal condition has been achieved!

Where does 116 come from?

This was supposed to be based on the label width from the mobile app which was 96px for the unpadded text. I've since moved the padding around and didn't readjust the formula here (because this column needs to contain padded text). I've replaced it with 96 + 8 to match the old width with the new padding values.

Neat. A bit tricky, though. Would definitely be good to have some tests for this — really just needs three cases, where the target user ID is equal to, less than, or greater than the self ID.

In particular I saw this code and was tempted to say it can simplify to [userId, selfUserId]..sort(), and only then remembered why we extracted it in the first place 🙂

nit: match ordering of fields' declarations

bump

nit: Flutter upstream style seems to be that the build method is always at the end of the class ­— a handy convention for quickly finding it, since that's often among the first things one wants to find. So then its various helper methods go above it.

nit:

to match the verb used for the other similar methods (and for functions returning a widget in general)

nit: _sortCustomProfileFields already allocates a List, so this makes an unnecessary copy; could either assign to customProfileFields rather than clear/addAll (and make it non-final to accommodate that), or drop the toList in _sortCustomProfileFields and have the other call site do toList for itself

I've changed customProfileFields to be non-final instead. It seems this is the first non-final member, but it doesn't seem a pattern to maintain as I think there are some other top-level settings that will eventually need to be non-final (like realm_message_content_edit_limit_seconds)

So this Table widget seems neat and is good to be aware of, but I'm wondering now if it's actually doing much for us in this case. It looks like we should be able to get the same effect with just a Column whose children are Rows; in each row use a SizedBox(width: …, child: …) to fix the width of the label and an Expanded to let the value take up the rest of the width.

I come to that thought from looking at the FixedColumnWidth(96 + 8) and noticing that the numbers still feel a bit mysterious; and in particular that the 8 is meant to correspond to the 8 down below in the padding, but they're separated by a good few lines of code that's talking about other things. The plain Row treatment would make it, in the first instance,

          SizedBox(width: 96 + 8,
            child: Padding(padding: const EdgeInsets.fromLTRB(0, 4, 8, 4),
              child: Text(entry.label, style: const TextStyle(fontWeight: FontWeight.bold)))),

so that those numbers are right next to each other; and then at that point the idiomatic thing would be to break it up further:

          SizedBox(width: 96,
            child: Padding(padding: const EdgeInsets.symmetric(vertical: 4),
              child: Text(entry.label, style: const TextStyle(fontWeight: FontWeight.bold)))),
          const SizedBox(width: 8),

at which point I think it's a good bit clearer what's going on.

Oh, and perhaps even a greater benefit of the plain Column/Row approach: that should let us drop all the 4px vertical padding on the individual labels and values, in favor of interspersing const SizedBox(height: 8) as children of the Column.

Together with the if (choiceFieldData == null) return null; line in my suggestion above, this can be compacted to:

(Copying from the zulip-mobile implementation.) This comment is basically another piece of the reverse-engineering results that substitute here for API docs. Even though it doesn't involve any arbitrary names (like choices, text, subtype, etc., as seen for some of the other CustomProfileFieldType values), it's information about the semantics of these types which wouldn't be obvious a priori.

This feels vertically pretty cramped to me:

Compare zulip-mobile:

(The difference in URL text is due to the bug I mentioned in a comment above where the arguments to _buildLinkWidget are swapped.)

I see that the source code in both implementations appears to call for the same 8px of spacing in between, though. Hmm — I suspect this TextStyle.height setting is the issue:

      style: const TextStyle(fontSize: 15, height: 1),

Aha, yep, if I remove both that and the height: 1.5 elsewhere, it gets more spacious. I think a typical TextStyle.height is more like 1.2, not 1.0.

The best solution here may be to just make less use of DefaultTextStyle — or at least to not use it for TextStyle.height. In particular the upper DefaultTextStyle is only applying to three Text widgets that I see, plus to _ProfileDataTable which overrides most of it; and of those three, one is a button label, which seems like it shouldn't necessarily get the same styles as the information text anyway.

I'll remove it from the profile data table, in the refactor away from the Table widget I'll have the Row builders apply whatever style to the labels. I think it's still useful to have it on the upper part, although it only has three Text widgets currently, there will be more fields to be added later when the features backing them are introduced.

Cool, that seems like a reasonable strategy. Though it looks like in the revision, you ended up doing away with the DefaultTextStyle entirely, in favor of having style constants (on a container class) and referring to those directly at Text widgets, and that approach looks good to me.

The InkWell needs some horizontal padding too ­— the highlighting looks odd at the left edge because it's flush with the avatar. (And if the user's name were just long enough to reach the right edge, I expect that'd look odd too.)

I've seen in other places usage of private _kFooBar variables used for reusable style following their 'Begin global constant names with prefix "k"' rule, but the same rule recommends using a container class over that. I do personally prefer this style, so I elected for refactor in this way for now.

Sure, this approach looks good to me.

nit: have test agree with tested code on name of constructor

nit:

(plus the other change)

/// A list of these is the decoding of values of [CustomProfileField.fieldData] when
/// the [CustomProfileField.type] is [CustomProfileFieldType.choice].

or that then sounds a bit awkward, so perhaps better:

bump

This checks these three elements are present but not that that's the whole map. Not a super high-risk thing to break, here, but it'd be nice to be complete.

One route is with deepEquals. That seems to end up a bit annoying with isA checks:

    check(choices).deepEquals({
      '0': it()..isA<CustomProfileFieldChoiceDataItem>().text.equals('Option 0'),
      '1': it()..isA<CustomProfileFieldChoiceDataItem>().text.equals('Option 1'),
      '2': it()..isA<CustomProfileFieldChoiceDataItem>().text.equals('Option 2'),
    });

though tolerable.

Another route would be deepEquals plus adding an == override.

Maybe the cleanest is our jsonEquals:

    check(choices).jsonEquals({
      '0': const CustomProfileFieldChoiceDataItem(text: 'Option 0'),
      '1': const CustomProfileFieldChoiceDataItem(text: 'Option 1'),
      '2': const CustomProfileFieldChoiceDataItem(text: 'Option 2'),
    });

bump #287 (comment)

It looks like some padding was added between the app bar and the error icon + text, which is probably good too. But the icon and the text look too close to each other.

Sure, this approach looks good to me.

This name is a bit unspecific — the whole page is the profile, so it doesn't help much in narrowing down which text this is for.

It seems like the answer is: currently name and role, and in the future perhaps email, user status, local time, etc. In other words the fields that are shown above the custom profile fields.

Perhaps headingText? Or primaryFieldText? Or could be profileHeadingText or profilePrimaryFieldText, though it's already private to the profile-page code so that doesn't seem essential.

Cool, that seems like a reasonable strategy. Though it looks like in the revision, you ended up doing away with the DefaultTextStyle entirely, in favor of having style constants (on a container class) and referring to those directly at Text widgets, and that approach looks good to me.